Skip to content

fix: support extraVolumes/extraVolumeMounts, fix WH CA handling (OP-388) - #2800

Open
kristina-solovyova wants to merge 1 commit into
09-10-docs_add_ainav_size_check_to_lint_trim_navigation_files_and_document_management_proxyfrom
09-07-fix_support_extravolumes_extravolumemounts_fix_wh_ca_handling_op-388_
Open

fix: support extraVolumes/extraVolumeMounts, fix WH CA handling (OP-388)#2800
kristina-solovyova wants to merge 1 commit into
09-10-docs_add_ainav_size_check_to_lint_trim_navigation_files_and_document_management_proxyfrom
09-07-fix_support_extravolumes_extravolumemounts_fix_wh_ca_handling_op-388_

Conversation

@kristina-solovyova

@kristina-solovyova kristina-solovyova commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Note

Medium Risk
Touches pod spec assembly, admission, and cluster-wide Weka Home CA behavior; misconfiguration can break TLS to Weka Home, though volume changes do not auto-roll pods.

Overview
Adds declarative extra volumes on WekaCluster (spec.podConfig), WekaClient (spec), propagated WekaContainers, and the operator Deployment via Helm manager.extraVolumes / extraVolumeMounts (with reserved tmpdir / /tmp). The pod factory applyExtraVolumes appends user volumes/mounts to the weka container only, with reserved-name/path checks and JSON normalization/digests so spec updates propagate without breaking HashStruct on CSI maps.

Admission registers cluster_extra_volumes and client_extra_volumes (strict JSON parse, reserved collisions) plus a warn rule client_wekahome_cacert_unverifiable for client-only topologies.

Weka Home TLS is documented and tightened: GetWekaHomeClientCacertSecret resolves CA secrets (client → same-namespace cluster → operator default), cross-namespace cluster secrets emit a throttled warning, and weka_runtime.py concatenates arbitrary Secret keys into the staged PEM only when real certificate content is present. Cluster/client upgrade paths now propagate extra volumes and resolved wekahome-cacert secrets to existing containers.

Reviewed by Cursor Bugbot for commit afac063. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary

  • extraVolumes/extraVolumeMounts on WekaCluster (spec.podConfig), WekaClient (spec), propagated to WekaContainers; mounts land on the weka container only. Admission validators reject reserved names, paths on/under/above operator mounts, and unknown fields.
  • Helm manager.extraVolumes/extraVolumeMounts for the operator Deployment (tmpdir//tmp reserved).
  • Weka Home CA: any Secret key name is staged, private-key entries are skipped, a certificate-less Secret stages nothing; clients inherit the same-namespace target cluster's cacertSecret, with a warning event for cross-namespace clusters and an admission warning for joinIpPorts clients.
  • Changing cacertSecret or extra volumes propagates to existing WekaContainer specs; the cluster-wide weka_cloud_ca_cert_path override is added/removed to match. Pods are not recreated automatically: delete a pod to apply the change (documented).

Breaking change

The deprecated, no-op spec.wekaHomeConfig field is removed from WekaClientSpec (API submodule bump). Manifests that still set it are now rejected by the API server's strict decoding (unknown field "spec.wekaHomeConfig"). Use spec.wekaHome instead.

Testing

Unit tests plus four lab cycles on a real cluster (formation, propagation, admission negatives, CA staging, client inheritance, manual pod recreation, override add/remove); see RESULTS-op388-lab.md in the branch.

🤖 Generated with Claude Code

kristina-solovyova commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label main-merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has required the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@kristina-solovyova
kristina-solovyova changed the base branch from main to graphite-base/2800 September 8, 2026 07:14
@kristina-solovyova
kristina-solovyova force-pushed the 09-07-fix_support_extravolumes_extravolumemounts_fix_wh_ca_handling_op-388_ branch from b28f878 to 5378268 Compare September 8, 2026 07:14
@kristina-solovyova
kristina-solovyova changed the base branch from graphite-base/2800 to 09-03-fix_tlc_drive_core_capping_and_address_linting_issues September 8, 2026 07:14
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

1 similar comment
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@kristina-solovyova
kristina-solovyova changed the base branch from 09-03-fix_tlc_drive_core_capping_and_address_linting_issues to graphite-base/2800 September 9, 2026 08:53
@kristina-solovyova
kristina-solovyova force-pushed the 09-07-fix_support_extravolumes_extravolumemounts_fix_wh_ca_handling_op-388_ branch from 5378268 to 651ba76 Compare September 9, 2026 11:39
@kristina-solovyova
kristina-solovyova changed the base branch from graphite-base/2800 to main September 9, 2026 11:39
@kristina-solovyova
kristina-solovyova marked this pull request as ready for review September 9, 2026 11:39
Copilot AI lite review requested due to automatic review settings September 9, 2026 11:39
@kristina-solovyova
kristina-solovyova requested a review from a team as a code owner September 9, 2026 11:39
@graphite-app
graphite-app Bot requested a review from tigrawap September 9, 2026 11:40
@graphite-app

graphite-app Bot commented Sep 9, 2026

Copy link
Copy Markdown

Graphite Automations

"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (09/09/26)

2 reviewers were added to this PR based on Anton Bykov's automation.

Comment thread internal/controllers/wekaclient/client_reconciler_loop.go Outdated
Comment on lines +849 to +854
// Propagate the resolved wekaHome cacertSecret so a change reaches already-running containers,
// not only ones created after the change (buildClientWekaContainer only runs on container-create).
if container.Spec.AdditionalSecrets["wekahome-cacert"] != newClientSpec.WekaHomeCacertSecret {
container.Spec.AdditionalSecrets = clientAdditionalSecrets(newClientSpec.WekaHomeCacertSecret)
changed = true
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacing the whole map means any other AdditionalSecrets key on the container is silently dropped, even though the if only tested the wekahome-cacert key. Today the map only ever has that one key (as the comment in extra_volumes.go:29-30 notes), so this is latent rather than broken — but it will break quietly the moment a second key is added, and the reserved-names list already depends on that same "exactly one entry" assumption in a second place.

Safer to mutate just the one key:

	if container.Spec.AdditionalSecrets["wekahome-cacert"] != newClientSpec.WekaHomeCacertSecret {
		if container.Spec.AdditionalSecrets == nil {
			container.Spec.AdditionalSecrets = map[string]string{}
		}
		if newClientSpec.WekaHomeCacertSecret == "" {
			delete(container.Spec.AdditionalSecrets, "wekahome-cacert")
		} else {
			container.Spec.AdditionalSecrets["wekahome-cacert"] = newClientSpec.WekaHomeCacertSecret
		}
		changed = true
	}

Also: "wekahome-cacert" is now a bare literal in three places (here, clientAdditionalSecrets, factory/container_factory.go:96) plus a derived "wekahome-cacert-secret" in resources.ReservedVolumeNames. Worth a single exported const so the reserved-name entry can be derived from it rather than hand-copied.

Comment thread internal/controllers/resources/extra_volumes.go
Comment thread internal/controllers/resources/extra_volumes.go
Comment on lines +3192 to +3210
if [ -d /var/run/secrets/weka-operator/wekahome-cacert ]; then
rm -rf /opt/weka/k8s-runtime/vars/wh-cacert
mkdir -p /opt/weka/k8s-runtime/vars/wh-cacert/
cp /var/run/secrets/weka-operator/wekahome-cacert/cert.pem /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem
chmod 400 /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem
# Secret data-key names are arbitrary, so concatenate every mounted PEM rather
# than assuming one is named cert.pem (the glob skips the ..data/..2025_* dotfiles).
for f in /var/run/secrets/weka-operator/wekahome-cacert/*; do
[ -f "$f" ] || continue
cat "$f" >> /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem
echo "" >> /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem
done
# Test for actual PEM content, not file size: the separator above writes a newline
# per key, so a secret holding only empty or non-PEM values still yields a non-empty
# file. An explicit CA replaces the system trust store, so pointing
# weka_cloud_ca_cert_path at a certificate-less file breaks Weka Home silently.
if grep -q "BEGIN CERTIFICATE" /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem 2>/dev/null; then
chmod 400 /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem
else
rm -rf /opt/weka/k8s-runtime/vars/wh-cacert
fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things here.

1. Concatenating every file in the secret dir will inline a private key if the user points cacertSecret at a kubernetes.io/tls secret. That's a very natural mistake — a TLS secret is the obvious thing to reach for, and it has tls.crt + tls.key. The BEGIN CERTIFICATE check passes (thanks to tls.crt), so tls.key lands verbatim inside /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem, which is then pointed at by weka_cloud_ca_cert_path. Worth filtering to certificate blocks, or at least skipping files containing a private key:

for f in /var/run/secrets/weka-operator/wekahome-cacert/*; do
    [ -f "$f" ] || continue
    grep -q "BEGIN CERTIFICATE" "$f" || continue
    grep -q "PRIVATE KEY" "$f" && continue
    cat "$f" >> /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem
    echo "" >> /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem
done

That also removes the need for the post-hoc content test, since a dir with no PEM certs produces no file at all.

2. chmod 400 happens only after the loop, so the file exists with the default umask (typically 0644) for the duration of the concatenation. Harmless for a public CA bundle, but combined with (1) it's a real window on a private key. Set the mode before writing:

: > /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem
chmod 400 /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem

(then drop the trailing chmod, keeping only the rm -rf on the no-certs path).

Nit on the comment: "the glob skips the ..data/..2025_* dotfiles" — true, but the reason is that shell globs don't match leading dots, not the date; the ..2025_* naming will read as stale in a year. # the glob skips the secret's ..data/..timestamp dotfiles is enough.

Comment on lines +59 to +100
## Reserved names and paths

The operator reserves certain volume names, name suffixes, and mount-path
prefixes for its own use inside weka/client pods. An `extraVolumes` entry or
`extraVolumeMounts` entry that collides is rejected — at admission time if
enabled, and always by the pod factory as a final backstop. The authoritative
lists live in
[`internal/controllers/resources/extra_volumes.go`](../../../internal/controllers/resources/extra_volumes.go).

**Reserved volume names** (`ReservedVolumeNames`) — every volume name the
operator itself assigns anywhere in a weka pod (backend, client, or init
container). Volumes are pod-scoped, so a name used only by an init container
is reserved too, even though extra mounts never land in init containers (see
[Mount scope](#mount-scope-weka-container-only) below):

```
osrelease, dev, run, sys, weka-boot-scripts, hugepages, smbw-shm,
host-shared-netns, weka-container-persistence-dir, weka-container-shared-dir,
weka-cluster-persistence-dir, weka-container-global-persistence-dir,
weka-proxy-socket-dir, weka-ssdproxy-local-socket, node-info, weka-credentials,
proc-sysrq-trigger, proc-cmdline, devenv, google-cloud-key, host-modules,
host-usr-src, shared-weka-version, otel-packages, libmodules, usrsrc,
gcloud-credentials, wekahome-cacert-secret
```

`wekahome-cacert-secret` is the one name in that list the operator derives at
runtime rather than hardcodes — it comes from `spec.additionalSecrets`, which
forms `<name>-secret`. Only that literal name is reserved, so an ordinary user
name like `corp-ca-secret` is fine.

**Reserved mount-path prefixes** (`ReservedMountPathPrefixes`) — a mount
cannot land on or under:

```
/dev, /sys, /host, /host-binds, /hostside, /opt/weka,
/opt/weka-global-persistence, /var/run/secrets/weka-operator,
/usr/local/bin/weka, /etc/wekaio, /etc/syslog-ng,
/shared-python-packages, /shared-weka-version, /var/log, /lib/modules,
/usr/src, /var/secrets/google
```

**Reserved exact paths** (`ReservedMountPaths`) — operator mounts that are

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section describes an API that doesn't exist, which will send readers looking for symbols they can't find.

  1. ReservedMountPathPrefixes (line 89) and "Reserved exact paths" (line 100)extra_volumes.go:38-50 has a single ReservedMountPaths slice, and IsReservedMountPath applies the same on-or-under rule to every entry. There is no prefixes/exact split. The distinction the code actually makes is that a file entry (/opt/weka_runtime.py) reserves only itself simply because nothing lives under it — not because it's matched differently.

  2. "name suffixes" (line 61) — no suffix reservation exists. extra_volumes.go:29-32 explicitly rejects that approach in favour of reserving the one literal derived name, which line 84-88 then correctly explains. The two statements contradict each other.

  3. Line 84 says wekahome-cacert-secret "comes from spec.additionalSecrets" — it's the WekaContainer's spec.additionalSecrets, which users don't set; the operator populates it from spec.wekaHome.cacertSecret. Worth saying so, since as written it reads like a user-facing field.

Suggest collapsing 89-100 into one "Reserved mount paths (ReservedMountPaths) — a mount cannot land on, or under, any of:" list with the full 13 entries, and dropping "name suffixes" from line 61.

|---|---|---|
| WekaCluster | `spec.wekaHome.cacertSecret` | Mounts the Secret into every backend pod, stages it at `/opt/weka/k8s-runtime/vars/wh-cacert/cert.pem`, and sets the cluster-wide `weka_cloud_ca_cert_path` from a drive container. |
| WekaClient | `spec.wekaHome.cacertSecret` | Places the same file on the client pod at the same path. When `targetCluster` is set and the cluster is in the **same namespace**, this is **derived automatically from the target cluster's own `cacertSecret`** — set it explicitly only to override that default. A cluster in another namespace is not inherited from: only the Secret *name* would be copied, and it would not resolve in the client's namespace, so the client emits a warning event and you must set the field yourself. |
| Operator (Helm) | `wekahome.cacertSecret` | Used only by the operator's own CR reporter (the process that periodically reports CRs to Weka Home). It reads the Secret through the Kubernetes API into an in-memory certificate pool at request time. **No volume, no mount, no container filesystem is involved.** |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Used only by the operator's own CR reporter … No volume, no mount, no container filesystem is involved" is not correct, including for the code in this PR.

The Helm wekahome.cacertSecret lands in env.Config.WekaHome.CacertSecret, and that is:

  • the last-resort fallback returned by GetWekaHomeClientCacertSecret (internal/pkg/domain/wekahome.go:86), which this PR wires into AdditionalSecrets → a secret volume + mount on every client pod;
  • copied into WekaHomeConfig.CacertSecret by GetWekahomeConfig (wekahome.go:32-34), which container_factory.go:87-98 turns into additionalSecrets["wekahome-cacert"] → a secret volume + mount on every backend pod.

So it is a genuine cluster-wide/client-wide default that is mounted, not a reporter-only knob. Someone reading this table would set the Helm value expecting no pod impact and get every weka pod remounted (and rolled, once the pod-config-hash gap is fixed).

Also on line 85 (WekaClient row): the precedence chain is described as client → same-namespace cluster, but omits the third step. Per wekahome.go:79-87 it's actually:

  1. wekaClient.spec.wekaHome.cacertSecret
  2. target cluster's spec.wekaHome.cacertSecret, only if same namespace
  3. Helm wekahome.cacertSecret (the operator-wide default)

Worth stating all three explicitly, since (3) is what a cross-namespace client silently falls back to after the warning event.

One more omission worth a sentence somewhere in this file: weka_runtime.py:3202-3210 deletes the staged wh-cacert directory when the concatenated file contains no BEGIN CERTIFICATE. That's a deliberate and good safety valve, but from the operator's point of view it's a silent fall back to the OS trust store — the pod starts fine and nothing surfaces the misconfigured Secret. Readers debugging "I set cacertSecret and nothing changed" need to know to check that file's existence.

Comment on lines +70 to +87
clusterSecret := ""
if sameNamespace && targetCluster.Spec.WekaHome != nil {
clusterSecret = targetCluster.Spec.WekaHome.CacertSecret
}

if clientSecret != "" {
return clientSecret, false
}
if clusterSecret != "" {
return clusterSecret, false
}

if !sameNamespace && targetCluster != nil && targetCluster.Spec.WekaHome != nil && targetCluster.Spec.WekaHome.CacertSecret != "" {
crossNamespaceSkipped = true
}

return env.Config.WekaHome.CacertSecret, crossNamespaceSkipped
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crossNamespaceSkipped return is only consumed in buildClientWekaContainer, which runs on container creation (client_reconciler_loop.go:485-495). NewUpdatableClientSpec — the path taken on every reconcile of an existing client — discards it with whCaCert, _ := … (line 1377). So the "you must set cacertSecret yourself" warning event never reaches the users most likely to need it: someone who adds spec.wekaHome.cacertSecret to an existing cross-namespace cluster and wonders why their clients didn't pick it up. Emitting it from the reconcile path (still throttled) would fix that.

Small cleanup in the same function: the final if re-derives what sameNamespace already told you. Since clusterSecret is only populated when sameNamespace, reaching line 84 with a non-empty cluster secret implies !sameNamespace:

	if clientSecret != "" {
		return clientSecret, false
	}
	if clusterSecret != "" {
		return clusterSecret, false
	}
	// Only reachable with !sameNamespace, since clusterSecret is populated only when same-namespace.
	if targetCluster != nil && targetCluster.Spec.WekaHome != nil && targetCluster.Spec.WekaHome.CacertSecret != "" {
		crossNamespaceSkipped = true
	}
	return env.Config.WekaHome.CacertSecret, crossNamespaceSkipped

Also worth noting for the test suite: wekahome_test.go:70-79 uses same-secret for both the client and the cluster value, so it can't distinguish "client wins" from "cluster wins" — it passes under either precedence. Use distinct values there.

Comment on lines +66 to 72
{{- range .Values.manager.extraVolumes }}
{{- if eq .name "tmpdir" }}{{ fail "manager.extraVolumes: the volume name \"tmpdir\" is reserved by the operator" }}{{ end }}
{{- end }}
{{- range .Values.manager.extraVolumeMounts }}
{{- if eq .mountPath "/tmp" }}{{ fail "manager.extraVolumeMounts: the mount path \"/tmp\" is reserved by the operator" }}{{ end }}
{{- end }}
volumes:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The /tmp guard is exact-match only, so mountPath: /tmp/foo (or /tmp/) sails through and shadows part of the operator's own tmpdir emptyDir. Same on-or-under rule the Go side uses (IsReservedMountPath) would be more consistent:

      {{- range .Values.manager.extraVolumeMounts }}
      {{- if or (eq .mountPath "/tmp") (hasPrefix "/tmp/" .mountPath) }}{{ fail "manager.extraVolumeMounts: the mount path \"/tmp\" is reserved by the operator" }}{{ end }}
      {{- end }}

Also nothing here catches duplicate names or duplicate mount paths within the user's own lists — Kubernetes will reject the Deployment, but with a much less obvious message than the two fails above. Low priority given the failure is at least loud.

Comment thread internal/validation/extra_volumes.go
Comment thread doc/api_dump/wekaclient.md
Comment thread internal/controllers/wekaclient/client_reconciler_loop_test.go Outdated
Copilot AI review requested due to automatic review settings September 10, 2026 09:56
@kristina-solovyova
kristina-solovyova force-pushed the 09-07-fix_support_extravolumes_extravolumemounts_fix_wh_ca_handling_op-388_ branch from 651ba76 to 36e9427 Compare September 10, 2026 09:56

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new admission validation code has a confirmed nil-map panic path when extraVolumeMounts is set without extraVolumes, which must be fixed before merge.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

doc/operator/deployment/extra-volumes.md:90

  • The docs reference ReservedMountPathPrefixes, but the code defines a single ReservedMountPaths list and IsReservedMountPath checks both exact matches and subpaths. Using the wrong identifier makes the documentation harder to follow/search.
**Reserved mount-path prefixes** (`ReservedMountPathPrefixes`) — a mount
cannot land on or under:

doc/summary.xml:62

  • This tag list mentions ReservedMountPathPrefixes, but the implementation uses ReservedMountPaths (see internal/controllers/resources/extra_volumes.go). Keeping the identifier consistent helps cross-referencing and search.
    <file relpath="operator/deployment/extra-volumes.md">
        <tags>extraVolumes, extraVolumeMounts, podConfig, manager.extraVolumes, RawExtension, schemaless, VolumeMount, cluster_extra_volumes, client_extra_volumes, ReservedVolumeNames, ReservedMountPathPrefixes, DisallowUnknownFields, pod-config-version, podConfigHash, allowRotateNonAnnotatedPodConfigHash, upgradePolicy, CA bundle, tmpdir</tags>
        <summary>Documents the three extra-volumes surfaces — WekaCluster spec.podConfig.extraVolumes/extraVolumeMounts (all pods of the cluster), WekaClient spec.extraVolumes/extraVolumeMounts (all client pods, flat spec, no podConfig), and the operator's own Helm manager.extraVolumes/extraVolumeMounts — for mounting arbitrary volumes (e.g. a private CA bundle) without a custom image. Explains that extraVolumes is a schemaless RawExtension while extraVolumeMounts is a typed VolumeMount list, and the resulting gap that only admission's DisallowUnknownFields decoding (cluster_extra_volumes/client_extra_volumes, Error at both severities) catches typos when the API server itself will not. Lists the reserved volume names/suffixes and mount-path prefixes (noting /etc/ssl and /etc/pki are deliberately not reserved), and that mounts land on the weka container only, never init containers. Covers propagation from WekaCluster/WekaClient onto owned WekaContainer specs, and the pod-config-hash rollout mechanics: pods are deleted and recreated (never patched) when the hash changes, unused digests are omitempty so non-adopters see no churn on operator upgrade, and pods that predate pod-config-hash tracking are adopted without rolling — so the first extraVolumes change on such a pod silently does nothing until it's deleted once or allowRotateNonAnnotatedPodConfigHash is set. Includes worked YAML for a cluster CA-bundle mount, a client-only (joinIpPorts, no targetCluster) OS-trust-store mount, and manager.extraVolumes for the operator's own Deployment.</summary>
  • Files reviewed: 41/41 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread internal/validation/extra_volumes.go
Copilot AI review requested due to automatic review settings September 10, 2026 10:15
@kristina-solovyova
kristina-solovyova force-pushed the 09-07-fix_support_extravolumes_extravolumemounts_fix_wh_ca_handling_op-388_ branch from 36e9427 to 1b9c8ea Compare September 10, 2026 10:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

applyExtraVolumes currently doesn’t validate mountPath absolute/clean as a backstop when admission is disabled, which can lead to hard-to-diagnose Pod creation failures.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

doc/operator/deployment/extra-volumes.md:90

  • This section references ReservedMountPathPrefixes, but the code exposes only ReservedMountPaths (which is used for both directory-prefix reservations and exact file paths). Keeping the identifier accurate helps readers find the source of truth in internal/controllers/resources/extra_volumes.go.
**Reserved mount-path prefixes** (`ReservedMountPathPrefixes`) — a mount
cannot land on or under:

doc/summary.xml:62

  • The tags list includes ReservedMountPathPrefixes, but the implementation uses ReservedMountPaths (see internal/controllers/resources/extra_volumes.go). Updating the tag keeps search keywords aligned with the actual API/implementation names.
    <file relpath="operator/deployment/extra-volumes.md">
        <tags>extraVolumes, extraVolumeMounts, podConfig, manager.extraVolumes, RawExtension, schemaless, VolumeMount, cluster_extra_volumes, client_extra_volumes, ReservedVolumeNames, ReservedMountPathPrefixes, DisallowUnknownFields, pod-config-version, podConfigHash, allowRotateNonAnnotatedPodConfigHash, upgradePolicy, CA bundle, tmpdir</tags>
        <summary>Documents the three extra-volumes surfaces — WekaCluster spec.podConfig.extraVolumes/extraVolumeMounts (all pods of the cluster), WekaClient spec.extraVolumes/extraVolumeMounts (all client pods, flat spec, no podConfig), and the operator's own Helm manager.extraVolumes/extraVolumeMounts — for mounting arbitrary volumes (e.g. a private CA bundle) without a custom image. Explains that extraVolumes is a schemaless RawExtension while extraVolumeMounts is a typed VolumeMount list, and the resulting gap that only admission's DisallowUnknownFields decoding (cluster_extra_volumes/client_extra_volumes, Error at both severities) catches typos when the API server itself will not. Lists the reserved volume names/suffixes and mount-path prefixes (noting /etc/ssl and /etc/pki are deliberately not reserved), and that mounts land on the weka container only, never init containers. Covers propagation from WekaCluster/WekaClient onto owned WekaContainer specs, and the pod-config-hash rollout mechanics: pods are deleted and recreated (never patched) when the hash changes, unused digests are omitempty so non-adopters see no churn on operator upgrade, and pods that predate pod-config-hash tracking are adopted without rolling — so the first extraVolumes change on such a pod silently does nothing until it's deleted once or allowRotateNonAnnotatedPodConfigHash is set. Includes worked YAML for a cluster CA-bundle mount, a client-only (joinIpPorts, no targetCluster) OS-trust-store mount, and manager.extraVolumes for the operator's own Deployment.</summary>
  • Files reviewed: 41/41 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread internal/controllers/resources/pod.go
Copilot AI review requested due to automatic review settings September 11, 2026 12:50
@kristina-solovyova
kristina-solovyova force-pushed the 09-07-fix_support_extravolumes_extravolumemounts_fix_wh_ca_handling_op-388_ branch from 1b9c8ea to 4f3ba94 Compare September 11, 2026 12:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Unresolved critical and moderate findings affect volume safety and Weka Home CA propagation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

charts/weka-operator/resources/weka_runtime.py:3233

  • Removing the staged directory here does not stop SetWekaHome from configuring weka_cloud_ca_cert_path: that code tests only whether the Secret name is non-empty. For an empty or non-PEM Secret, this branch therefore leaves Weka configured to a nonexistent CA file instead of falling back to the OS trust store (or failing validation). The runtime needs to communicate that no usable CA was staged, or the override must be cleared/not set.
            if grep -q "BEGIN CERTIFICATE" /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem 2>/dev/null; then
                chmod 400 /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem
            else
                rm -rf /opt/weka/k8s-runtime/vars/wh-cacert
            fi

charts/weka-operator/templates/manager.yaml:532

  • Unlike the WekaCluster/WekaClient validation, this template does not verify that each extra mount name belongs to manager.extraVolumes. A typo or intentional use of name: tmpdir therefore mounts the operator-managed emptyDir at an arbitrary second path instead of rejecting the configuration (the CR validator explicitly rejects this case). Validate mount names against the user-supplied volumes and reserve operator-managed volume references.
            {{- with .Values.manager.extraVolumeMounts }}
            {{- toYaml . | nindent 12 }}
            {{- end }}

doc/operator/deployment/extra-volumes.md:89

  • The identifier here does not exist: the implementation exports ReservedMountPaths and has no ReservedMountPathPrefixes. This makes the authoritative lookup instruction misleading; use the implementation's actual identifier.
**Reserved mount-path prefixes** (`ReservedMountPathPrefixes`) — a mount
  • Files reviewed: 37/37 changed files
  • Comments generated: 9
  • Review effort level: Lite

Comment on lines +69 to +70
{{- range .Values.manager.extraVolumeMounts }}
{{- if eq .mountPath "/tmp" }}{{ fail "manager.extraVolumeMounts: the mount path \"/tmp\" is reserved by the operator" }}{{ end }}
Comment on lines +66 to +74
// IsReservedMountPath reports whether p is, or falls under, an operator-managed mount path.
func IsReservedMountPath(p string) bool {
clean := path.Clean(p)
for _, reserved := range ReservedMountPaths {
if clean == reserved || strings.HasPrefix(clean, reserved+"/") {
return true
}
}
return false
Comment on lines +1004 to +1008
cleanPath := path.Clean(m.MountPath)
if IsReservedMountPath(cleanPath) {
return fmt.Errorf("extraVolumeMounts: %q is a reserved mount path", m.MountPath)
}
if _, exists := existingPaths[cleanPath]; exists {
Comment on lines +840 to +842
if container.Spec.AdditionalSecrets["wekahome-cacert"] != newClientSpec.WekaHomeCacertSecret {
container.Spec.AdditionalSecrets = domain.WekaHomeAdditionalSecrets(newClientSpec.WekaHomeCacertSecret)
changed = true
Comment on lines +390 to +392
if container.Spec.AdditionalSecrets["wekahome-cacert"] != updatableSpec.WekaHomeCacertSecret {
container.Spec.AdditionalSecrets = domain.WekaHomeAdditionalSecrets(updatableSpec.WekaHomeCacertSecret)
}
Comment on lines +3215 to 3217
if [ -d /var/run/secrets/weka-operator/wekahome-cacert ]; then
rm -rf /opt/weka/k8s-runtime/vars/wh-cacert
mkdir -p /opt/weka/k8s-runtime/vars/wh-cacert/
chmod 400 /opt/weka/k8s-runtime/vars/wh-cacert/cert.pem
# Secret data-key names are arbitrary, so concatenate every mounted PEM rather
# than assuming one is named cert.pem (the glob skips the ..data/..2025_* dotfiles).
for f in /var/run/secrets/weka-operator/wekahome-cacert/*; do
Comment thread internal/controllers/wekaclient/client_reconciler_loop.go
ExtraVolumesDigest: resources.ExtraVolumesDigest(clusterForHp.GetRawExtraVolumes()),
ExtraVolumeMounts: clusterForHp.GetExtraVolumeMounts(),
ExtraVolumeMountsDigest: resources.ExtraVolumeMountsDigest(clusterForHp.GetExtraVolumeMounts()),
WekaHomeCacertSecret: domain.GetWekaHomeClusterCacertSecret(clusterForHp),
@kristina-solovyova
kristina-solovyova changed the base branch from main to graphite-base/2800 September 11, 2026 13:13
@kristina-solovyova
kristina-solovyova force-pushed the 09-07-fix_support_extravolumes_extravolumemounts_fix_wh_ca_handling_op-388_ branch from 4f3ba94 to afac063 Compare September 11, 2026 13:14
@kristina-solovyova
kristina-solovyova changed the base branch from graphite-base/2800 to 09-10-docs_add_ainav_size_check_to_lint_trim_navigation_files_and_document_management_proxy September 11, 2026 13:14

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit afac063. Configure here.


if container.Spec.AdditionalSecrets["wekahome-cacert"] != updatableSpec.WekaHomeCacertSecret {
container.Spec.AdditionalSecrets = domain.WekaHomeAdditionalSecrets(updatableSpec.WekaHomeCacertSecret)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clearing CA secret leaves path set

Medium Severity

Clearing spec.wekaHome.cacertSecret now rewrites AdditionalSecrets to an empty map so replacement pods no longer stage a PEM, while SetWekaHome only adds weka_cloud_ca_cert_path and never removes it. After a documented pod recreate, that path still replaces the OS trust store and Weka Home TLS fails cluster-wide.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit afac063. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants